-
Notifications
You must be signed in to change notification settings - Fork 21
compute mu #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
compute mu #278
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 496 500 +4
=========================================
+ Hits 496 500 +4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline
src/diffpy/utils/tools.py
Outdated
|
||
Parameters | ||
---------- | ||
sample str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would normally do tihs as:
sample: str
The chemical formula or the name of the material
i.e., colon and sentencecse
src/diffpy/utils/tools.py
Outdated
the chemical formula or the name of the material | ||
energy float | ||
the energy in eV | ||
density float or None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
density: float, optional, Default is None
src/diffpy/utils/tools.py
Outdated
energy float | ||
the energy in eV | ||
density float or None | ||
material density in gr/cm^3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be "sample mass density" or even better, "mass density of the packed powder/sample" to be clear. the material desnsity is something quite different (mass of atoms in the unit cell divided by unit cell volume)
src/diffpy/utils/tools.py
Outdated
@@ -131,3 +133,25 @@ def get_package_info(package_names, metadata=None): | |||
pkg_info.update({package: importlib.metadata.version(package)}) | |||
metadata["package_info"] = pkg_info | |||
return metadata | |||
|
|||
|
|||
def compute_mu_using_xraydb(sample, energy, density=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's change sample
to composition
or maybe sample_composition
.
I think we probably need packing_fraction
as an optional arg.
tests/test_tools.py
Outdated
@@ -189,3 +189,9 @@ def test_get_package_info(monkeypatch, inputs, expected): | |||
) | |||
actual_metadata = get_package_info(inputs[0], metadata=inputs[1]) | |||
assert actual_metadata == expected | |||
|
|||
|
|||
def test_compute_mu_using_xraydb(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a test for no density passed in. Also for packing fraction passed in and also for both passed in. Think about what behavior you want in each of these cases and write a "case" (look elsewhere in test_tools for how we are handling this now)
@sbillinge ready for another review |
src/diffpy/utils/tools.py
Outdated
@@ -131,3 +133,31 @@ def get_package_info(package_names, metadata=None): | |||
pkg_info.update({package: importlib.metadata.version(package)}) | |||
metadata["package_info"] = pkg_info | |||
return metadata | |||
|
|||
|
|||
def compute_mu_using_xraydb(sample_composition, energy, density=None, packing_fraction=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added packing fraction. does this make sense or should we use None instead? I think if we set the default to 1 the code can be more concise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting to None is clearer here. The logic from the perspective of the user is that we either have a packing fraction or we don't. If it is None, you can set it to 1 in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, didn't we discuss to change the name to sample_mass_density
?
tests/test_tools.py
Outdated
({"sample_composition": "H2O", "energy": 10000, "density": 0.997, "packing_fraction": 1}, 0.5330), | ||
({"sample_composition": "H2O", "energy": 10000, "density": 0.4985, "packing_fraction": 1}, 0.2665), | ||
# C4: user specified a standard density and a packing fraction | ||
({"sample_composition": "H2O", "energy": 10000, "density": 0.997, "packing_fraction": 0.5}, 0.2665), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added test cases with description. I didn't include the bad cases where the user does not specify a density and xraydb doesn't know the density (this is taken care by xraydb)
tests/test_tools.py
Outdated
sample, energy, density = "ZrO2", 17445.362740959618, 1.009 | ||
actual_mu = compute_mu_using_xraydb(sample, energy, density=density) | ||
assert actual_mu == pytest.approx(1.252, rel=1e-4, abs=0.1) | ||
params_mu = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please follow the guides provided here? https://gitlab.thebillingegroup.com/resources/group-wiki/-/wikis/Group-standards-for-pytest-and-docstrings
add higher level function purpose, use descriptive yet concise variable names, no need to create extra variable of params_mu
, no need to add "user specific, etc."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge if you have also further suggestions to the gitlab doc, please suggest or modify directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gitlab doc looks pretty good to me. Lot's of great examples to work from.
thanks @bobleesj the link is very helpful and very clear to me. I edited the test comments. let me know if anything is unclear! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline comments. ATM I am just reviewing tests until we get them sorted. I don't really know what "standard density", and it doesn't make too much sense to me, so could you elaborate on that?
tests/test_tools.py
Outdated
], | ||
) | ||
def test_compute_mu_using_xraydb(inputs, expected_mu): | ||
actual_mu = compute_mu_using_xraydb( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you put
def test_compute_mu_using_xraydb(inputs, expected_mu):
actual_mu = compute_mu_using_xraydb(**inputs)
which will unpack the input dict. You may have to handle the required args differently, but you can then have input_args
and input_kwargs
in your paramatrize and unpack them differently.
tests/test_tools.py
Outdated
"inputs, expected_mu", | ||
[ | ||
# Test whether the function returns the correct mu | ||
( # C1: No density or packing fraction provided, expect to compute mu based on standard density |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to allow this? what is "standard density"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just density I think - density under the standard conditions. So user can specify either a measured density or packing fraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I'm not putting a strict "and/or" for density and packing fraction is that xraydb does not know the density to some materials like ZrO2. In this case user needs to put a density and a packing fraction, or they can put their measured density.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so density is in the database? that is unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does xraydb compute the density?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it has a database of some standard materials. So we will need tests for cases where the material can be found and cases where it can't. How many different materials are in that database? The examples show just a few satandard materials (Kapton, quartz, etc.). Is it an extensive database or just a few x-ray standards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will you compute it for the cases where it is not in the db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have densities for some materials (for example, this returns sample name and density: https://xraypy.github.io/XrayDB/python.html#xraydb.get_materials).
I just reread their docstring and their density refers to the material density, not mass density: https://xraypy.github.io/XrayDB/python.html#xraydb.material_mu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see inline comments.
Also, this is conflicted
tests/test_tools.py
Outdated
"inputs, expected_mu", | ||
[ | ||
# Test whether the function returns the correct mu | ||
( # C1: No density or packing fraction provided, expect to compute mu based on standard density |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it has a database of some standard materials. So we will need tests for cases where the material can be found and cases where it can't. How many different materials are in that database? The examples show just a few satandard materials (Kapton, quartz, etc.). Is it an extensive database or just a few x-ray standards?
tests/test_tools.py
Outdated
"inputs, expected_mu", | ||
[ | ||
# Test whether the function returns the correct mu | ||
( # C1: No density or packing fraction provided, expect to compute mu based on standard density |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will you compute it for the cases where it is not in the db?
So I think it only has density of pure elements and common compounds. I used I read through
It looks like it's using mass density so we're good. I'll solve the conflict and add more tests for unknown materials. |
src/diffpy/utils/tools.py
Outdated
sample_composition : str | ||
The chemical formula or the name of the material. | ||
energy : float | ||
The energy in keV. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change unit to keV
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The energy of the incident x-rays in keV"
src/diffpy/utils/tools.py
Outdated
|
||
Computes mu based on the sample composition and energy. | ||
User can provide a measured density or an estimated packing fraction. | ||
Specifying the density is recommended, though not required for some pure or standard materials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using xrayDB functionality here is not useful. The number of materials is so small, so there is no point to offer this to users as an option. It will just confuse them.
We could offer other approaches:
- given a composition, try and get density from COD or Materials Project. For different polymorphs the densities will be different for the same composition, so we would have to handle that.
- as a fallback, use a "best effort" atomic density (most tightly packed materials are around 5x10^22 cm^-3)? This would be "magic" which we don't normally like, but if we use this fallback we could give a warning.
tests/test_tools.py
Outdated
}, | ||
0.5330, | ||
), | ||
( # 2. Unknown material |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add unknown material for testing
@sbillinge ready for another review |
tests/test_tools.py
Outdated
], | ||
) | ||
def test_compute_mu_using_xraydb(inputs, expected_mu): | ||
actual_mu = compute_mu_using_xraydb(**inputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is beautiful. thanks! @yucongalicechen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline comments.
src/diffpy/utils/tools.py
Outdated
@@ -131,3 +133,31 @@ def get_package_info(package_names, metadata=None): | |||
pkg_info.update({package: importlib.metadata.version(package)}) | |||
metadata["package_info"] = pkg_info | |||
return metadata | |||
|
|||
|
|||
def compute_mu_using_xraydb(sample_composition, energy, density=None, packing_fraction=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting to None is clearer here. The logic from the perspective of the user is that we either have a packing fraction or we don't. If it is None, you can set it to 1 in the code.
src/diffpy/utils/tools.py
Outdated
@@ -131,3 +133,31 @@ def get_package_info(package_names, metadata=None): | |||
pkg_info.update({package: importlib.metadata.version(package)}) | |||
metadata["package_info"] = pkg_info | |||
return metadata | |||
|
|||
|
|||
def compute_mu_using_xraydb(sample_composition, energy, density=None, packing_fraction=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, didn't we discuss to change the name to sample_mass_density
?
src/diffpy/utils/tools.py
Outdated
|
||
Computes mu based on the sample composition and energy. | ||
User can provide a measured density or an estimated packing fraction. | ||
Specifying the density is recommended, though not required for some pure or standard materials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using xrayDB functionality here is not useful. The number of materials is so small, so there is no point to offer this to users as an option. It will just confuse them.
We could offer other approaches:
- given a composition, try and get density from COD or Materials Project. For different polymorphs the densities will be different for the same composition, so we would have to handle that.
- as a fallback, use a "best effort" atomic density (most tightly packed materials are around 5x10^22 cm^-3)? This would be "magic" which we don't normally like, but if we use this fallback we could give a warning.
src/diffpy/utils/tools.py
Outdated
sample_composition : str | ||
The chemical formula or the name of the material. | ||
energy : float | ||
The energy in keV. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The energy of the incident x-rays in keV"
tests/test_tools.py
Outdated
}, | ||
0.5330, | ||
), | ||
( # C2: Packing fraction (=0.5) provided only (only for known material) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse my pattern from C1 here.
tests/test_tools.py
Outdated
}, | ||
0.2665, | ||
), | ||
( # C3: Density provided only, expect to compute mu based on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse my pattern above.
tests/test_tools.py
Outdated
}, | ||
1.252, | ||
), | ||
( # C4: Both density and packing fraction are provided, expect to compute mu based on both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't make any sense. It will result in two numbers, not one number. We probably want to raise an exception if both are provided.
tests/test_tools.py
Outdated
) | ||
def test_compute_mu_using_xraydb(inputs, expected_mu): | ||
actual_mu = compute_mu_using_xraydb(**inputs) | ||
assert actual_mu == pytest.approx(expected_mu, rel=0.01, abs=0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit too loose?
@yucongalicechen further to my review comments, I recommend not using the densities from the xrayDB at all and just computing using given densities. We can add another function that is |
Just to clarify - this means user must enter density or packing fraction right? Something like what this is doing https://11bm.xray.aps.anl.gov/absorb/absorb.php? I can make another issue on getting density. |
diffpy/diffpy.labpdfproc#146 (comment) - actually I meant to write this comment here in diffpy.utils @yucongalicechen |
Yes, either or and it triggers a different workflow. |
src/diffpy/utils/tools.py
Outdated
"Warning: Density is set to None if a packing fraction is specified, " | ||
"which may cause errors for some materials. " | ||
"We recommend specifying sample mass density for now. " | ||
"Auto-density calculation is coming soon." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a warning message for now. I set density to None if user specifies packing fraction. We can probably replace this with density=get_density_from_cloud(sample_composition)
. There're some instructions here on how to estimate the density from given chemical formula: https://11bm.xray.aps.anl.gov/absorb/absorb.php. I can try to look into this more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy if we know the structure, but I am pretty sure MP has material density as an attribute so a simple API call will give it. We will have to add users adding their API key to the global config.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Density is just number of atoms/unit cell volume which we know from composition and any CIF file we find, then converted to mass density with the formula, then multiplied by packing fraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code could be pretty reusable so I expect it will end up in diffpy.utils in the end (but in a future release)
@sbillinge ready for another review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline comments.
Let's leave the "materials project/COD" workflow for v3.7.0
to correctly handle that, the best way is to define the functions and their signatures now (so the API desn't change when we implement it) but raise NotImplmentedError
Then wrap the line where the function is called wrap it in a try-except clause and handle the NotImplementedError
, something like:
def get_density_from_the_cloud(composition, mp_token="":
"""
docstring with few words about intent and that it isn't implemented yet
""""
raise NotImplementedError
return
then let's say this function gets called in the code, we do this:
try:
material_density = get_density_from_cloud(composition)
except NotImplementedError:
warning.warn("some helpful message about retrying using densiy")
This is a much tidier way of encapsulating what we need to change when we do actually implement it.
src/diffpy/utils/tools.py
Outdated
mu = material_mu(sample_composition, energy * 1000, density=sample_mass_density, kind="total") / 10 | ||
else: | ||
warnings.warn( | ||
"Warning: Density is set to None if a packing fraction is specified, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't need the word warning here. Warnings.warn handles that.
src/diffpy/utils/tools.py
Outdated
"Please rerun specifying only one." | ||
) | ||
if sample_mass_density is not None: | ||
mu = material_mu(sample_composition, energy * 1000, density=sample_mass_density, kind="total") / 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it more readable, don't hardcode numbers but rather variables with obvious names.
src/diffpy/utils/tools.py
Outdated
"Warning: Density is set to None if a packing fraction is specified, " | ||
"which may cause errors for some materials. " | ||
"We recommend specifying sample mass density for now. " | ||
"Auto-density calculation is coming soon." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy if we know the structure, but I am pretty sure MP has material density as an attribute so a simple API call will give it. We will have to add users adding their API key to the global config.....
src/diffpy/utils/tools.py
Outdated
"Warning: Density is set to None if a packing fraction is specified, " | ||
"which may cause errors for some materials. " | ||
"We recommend specifying sample mass density for now. " | ||
"Auto-density calculation is coming soon." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Density is just number of atoms/unit cell volume which we know from composition and any CIF file we find, then converted to mass density with the formula, then multiplied by packing fraction
src/diffpy/utils/tools.py
Outdated
"Warning: Density is set to None if a packing fraction is specified, " | ||
"which may cause errors for some materials. " | ||
"We recommend specifying sample mass density for now. " | ||
"Auto-density calculation is coming soon." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code could be pretty reusable so I expect it will end up in diffpy.utils in the end (but in a future release)
src/diffpy/utils/tools.py
Outdated
|
||
|
||
def get_density_from_cloud(sample_composition, mp_token=""): | ||
"""Function to get material density from the MP database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new function - not implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to also go to the COD database (doesn't require a token).
@yucongalicechen for future commit msgs/issues, could we try using this standard? (also aded to gitlab doc) Billingegroup/scikit-package#212 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! Please see inline comments.
src/diffpy/utils/tools.py
Outdated
|
||
|
||
def get_density_from_cloud(sample_composition, mp_token=""): | ||
"""Function to get material density from the MP database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to also go to the COD database (doesn't require a token).
src/diffpy/utils/tools.py
Outdated
try: | ||
sample_mass_density = get_density_from_cloud(sample_composition) * packing_fraction | ||
except NotImplementedError: | ||
warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be an error not a warning, no? We need to interrupt execution.
Also, let's apologise... "So sorry, Density computation from composition is not implemented right now. We hope to have this implemented in the next release. Please rerun specifying a sample mass density.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I will add the COD database and change the error message
thanks Bob! Will use that in my future commit messages |
@sbillinge ready for another review! Thanks so much for the comments above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks. Please see small inline comments
tests/test_tools.py
Outdated
"inputs", | ||
[ | ||
# Test when the function raises ValueError | ||
# C1: Both mass density and packing fraction are provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To compactify we want to write this as follows....also, put what you expect, e.g.,
# Test when the function has invalid inputs
( # C1: Both mass density and packing fraction are provided, expect ValueError exception
{
"sample_composition": "quartz",
"energy": 10,
"sample_mass_density": 2.65,
"packing_fraction": 1,
}
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nearly there. Still trying to finalize the example for others.
tests/test_tools.py
Outdated
# C1: Both mass density and packing fraction are provided, expect ValueError exception | ||
( | ||
{ | ||
"sample_composition": "quartz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"quartz" is not a omposition, so we don't want this in the tests. It doesn't matter for the test, but let's not confuse readers. Please also fix this below. You can use SiO2 instead.
Also, please can you move the # C2 down a line inside the paren as I tried to illustrate before. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Will fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops there're conflicts - willl fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see inline
src/diffpy/utils/tools.py
Outdated
Parameters | ||
---------- | ||
sample_composition : str | ||
The chemical formula or the name of the material. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not the name, just chemical formula.
src/diffpy/utils/tools.py
Outdated
energy : float | ||
The energy of the incident x-rays in keV. | ||
sample_mass_density : float, optional, Default is None | ||
The mass density of the packed powder/sample in gr/cm^3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
g/cm*3, not gr/cm^3
nice @yucongalicechen ! |
closes #45
@sbillinge ready for review. I can add docs in the same PR once we're happy with the function